Skip to content

Conversation

@brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Sep 8, 2025

All apps/packages now extend a common tsconfig.base.json and only add project specific changes in their own configs.

@brijeshb42 brijeshb42 requested review from a team and Copilot September 8, 2025 06:11
@brijeshb42 brijeshb42 added the scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). label Sep 8, 2025
@oliviertassinari oliviertassinari temporarily deployed to typescript-restructure - mui-tools-public PR #666 September 8, 2025 06:12 — with Render Destroyed

This comment was marked as outdated.

@mui-bot
Copy link

mui-bot commented Sep 8, 2025

Bundle size report

Bundle Parsed size Gzip size
@base-ui-components/react 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR restructures the TypeScript configuration files to follow a hierarchical approach. A new base configuration file tsconfig.options.json is introduced that contains common TypeScript compiler options, which is then extended by other configuration files to avoid duplication.

  • Created a new base configuration file (tsconfig.options.json) with common TypeScript compiler options
  • Replaced duplicated configurations across packages and apps with extends patterns
  • Removed the redundant tsconfig.build.json file from the code-infra package

Reviewed Changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tsconfig.options.json New base configuration file with common TypeScript compiler options
tsconfig.js.json New configuration extending base options for JavaScript-enabled projects
tsconfig.node.json Simplified to extend tsconfig.js.json and added babel.config.* to includes
packages/docs-infra/tsconfig.json Updated to extend tsconfig.options.json and removed redundant options
packages/code-infra/tsconfig.json Simplified to extend tsconfig.js.json and updated includes
packages/code-infra/tsconfig.build.json Removed as mentioned in PR description
packages/code-infra/src/cli/typescript.mjs Added --declarationMap false flag to TypeScript command
packages/bundle-size-checker/tsconfig.json Simplified to extend tsconfig.js.json
packages/babel-plugin-resolve-imports/tsconfig.json Simplified to extend tsconfig.js.json
packages/babel-plugin-minify-errors/tsconfig.json Simplified to extend tsconfig.js.json with updated includes
package.json Added @types/babel__core dependency
apps/code-infra-dashboard/tsconfig.json Simplified to extend tsconfig.options.json
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

@oliviertassinari oliviertassinari temporarily deployed to typescript-restructure - mui-tools-public PR #666 September 12, 2025 18:56 — with Render Destroyed
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 15, 2025
@brijeshb42 brijeshb42 force-pushed the typescript-restructure branch from eda5add to 8c940ce Compare September 22, 2025 13:09
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 22, 2025
@brijeshb42
Copy link
Contributor Author

@Janpot Can I get a review on this ?

@Janpot
Copy link
Member

Janpot commented Sep 23, 2025

When it comes to modularizing tsconfig, I'm not 100% convinced a deep hierarchy is the best solution. See this comment as a reason for allowing arrays in extends. The idea being that you create multiple base configs, each describing an aspect of the environment which you then can freely compose in your app/package configurations. I don't know how that should look exactly, but I'm thinking like e.g. our build tool can export its tsconfig that sets module resolution rules. Maybe we can make a config for when the target is node, and one for when it is browsers. Such that e.g. our node tools extend from ['tsconfig.node.json'] as they're not compiled, but docs-infra from ['tsconfig.browser.json', 'tsconfig.build.json']. In essence aiming at avoiding extending a tsconfig that itselfs also extends.
See also https://github.com/tsconfig/bases

@oliviertassinari oliviertassinari temporarily deployed to typescript-restructure - mui-tools-public PR #666 September 26, 2025 12:24 — with Render Destroyed
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 30, 2025
@brijeshb42 brijeshb42 force-pushed the typescript-restructure branch from 8c940ce to a783068 Compare October 3, 2025 05:42
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 3, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 31 out of 34 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)

packages/code-infra/package.json:55

  • The test:copy script references bin/code-infra.mjs which no longer exists after moving the entry point to src/code-infra.mjs. This should be updated to node src/code-infra.mjs to match the new bin path.
    "test:copy": "rm -rf build && node bin/code-infra.mjs copy-files --glob \"src/cli/*.mjs\" --glob \"src/eslint/**/*.mjs:esm\""

packages/code-infra/package.json:140

  • The files array includes "bin" directory which no longer exists after moving the CLI entry point to src/code-infra.mjs. This entry should be removed since the bin file is now in the src directory which is already included in the files array.
    "bin",

"include": ["src/**/*"],
// removing "cli" since its going to run directly and not really imported anywhere
"exclude": ["**/*.spec.*", "**/*.test.*", "**/setupVitest.ts"]
"exclude": ["**/*.spec.*", "**/*.test.*"]
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment explaining that CLI files are excluded because they run directly was removed, but the actual exclusion logic changed. The old config excluded setupVitest.ts explicitly; the new one only excludes test files. This could lead to setupVitest.ts being included in the build output unintentionally.

Suggested change
"exclude": ["**/*.spec.*", "**/*.test.*"]
"exclude": ["**/*.spec.*", "**/*.test.*", "setupVitest.ts"]

Copilot uses AI. Check for mistakes.
"build",
"src",
"README.md",
"!**/*.tsbuildinfo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, didn't know this supported negative patterns

@@ -1,11 +1,6 @@
{
"extends": "../../tsconfig.json",
"extends": ["../../tsconfig.base.json", "../../tsconfig.node.json"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Since we're bundling the code in docs-infra I'd expected it to have a different moduleResiolution (bundler) than what we'd use in untranspiled code for node (node16)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference between bundler and node16 is that bundler doesn't require extensions for relative imports.
I've updated tsconfig.node.json to have moduleResolution of bundler itself.

Copy link
Member

@Janpot Janpot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference between bundler and node16 is that bundler doesn't require extensions for relative imports.

It also resolves index.js files and probably a few other intricacies. There may also be other options that have to differ between transpiled and untranspiled code. e.g. lib, or target

I've updated tsconfig.node.json to have moduleResolution of bundler itself.

Bundler doesn't correctly reflect untranspiled code. If code is meant to run in node.js untranspiled than it needs to have the node16 moduleResolution flag set for it. This prevents us from writing code that wouldn't run. The Bundler option allows us to be ore lenient since the bundler fixes up the resolution for us.

All apps/packages now extend a common tsconfig.options.json and only add
project specific changes in their own configs.

Removed tsconfig.build.json in code-infra since there is no build step
tehre.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants